test_runner: warn on test file patterns that do not exist#64345
test_runner: warn on test file patterns that do not exist#64345UditDewan wants to merge 1 commit into
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
| if (hasUserSuppliedPattern) { | ||
| if (results.length === 0 && ArrayPrototypeEvery(glob.matchers, (m) => !m.hasMagic())) { | ||
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | ||
| process.exit(kGenericUserError); | ||
| } | ||
|
|
||
| const missing = ArrayPrototypeFilter(patterns, (pattern, i) => { | ||
| return !glob.matchers[i].hasMagic() && !existsSync(resolve(cwd, pattern)); | ||
| }); | ||
|
|
||
| if (missing.length > 0) { | ||
| console.error(`Could not find '${ArrayPrototypeJoin(missing, ', ')}'`); | ||
| process.exit(kGenericUserError); | ||
| } |
There was a problem hiding this comment.
| if (hasUserSuppliedPattern) { | |
| if (results.length === 0 && ArrayPrototypeEvery(glob.matchers, (m) => !m.hasMagic())) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } | |
| const missing = ArrayPrototypeFilter(patterns, (pattern, i) => { | |
| return !glob.matchers[i].hasMagic() && !existsSync(resolve(cwd, pattern)); | |
| }); | |
| if (missing.length > 0) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(missing, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } | |
| if (hasUserSuppliedPattern && results.length === 0) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } |
Wouldn't this suffice?
There was a problem hiding this comment.
Unfortunately not — that check only fires when all patterns collectively match nothing, which is the pre-existing behavior (minus the hasMagic() guard). In the repro from #64109, spec/*.test.js does match files, so results.length > 0 and no-tests-here is still silently dropped. The per-pattern check is what catches a nonexistent literal path even when other patterns match.
I also deliberately kept the hasMagic() guard on both checks so a glob that matches zero files keeps its current no-error behavior — removing it would newly fail runs that pass optional globs. Happy to make globs error too if that's preferred, but it seemed like a separate (and riskier) behavior change.
|
Not sure this is the direction proposed in nodejs/test-runner#13 but this would probably be a
semver-major
|
Emit a warning for each literal positional argument that does not match an existing file instead of silently dropping it. A warning rather than an error keeps existing passing invocations passing. Fixes: nodejs#64109
UditDewan
force-pushed
the
fix-test-runner-missing-patterns
branch
from
e2d29cb to
a8895e4
Compare
UditDewan
changed the title
|
@atlowChemi I've reworked this to emit a process warning instead of erroring, since #64109 asks for "a warning or error". Existing invocations keep their exit codes, so this should now be semver-minor, and it doesn't preclude whatever direction nodejs/test-runner#13 lands on for the broader --test parsing redesign. |
4 participants
Currently
createTestFileList()only errors when all user-supplied patterns collectively match nothing and none of them contain glob magic. As a result, a nonexistent literal path passed alongside a matching glob is silently dropped:This change adds a per-pattern check: any literal (non-glob) pattern that does not exist on disk is now reported with a process warning (
Warning: Could not find '...'). The run itself is unaffected — matching patterns still execute and the exit code is unchanged — so this avoids the semver-major implications of erroring (see review discussion) while still surfacing the mistake, which is what #64109 asks for ("a warning or error should be printed").Glob patterns that match nothing intentionally stay silent so existing workflows with optional globs keep working, and the pre-existing hard error when all patterns are literal and match nothing is preserved as-is.
Added test cases to
test/parallel/test-runner-cli.jscovering both repros from the issue, for both--test-isolation=noneandprocess.Fixes: #64109